Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: application emojis #1209

Closed
wants to merge 15 commits into from
Closed

Conversation

Mantouisyummy
Copy link

@Mantouisyummy Mantouisyummy commented Jul 19, 2024

Summary

Implements discord/discord-api-docs#7010

This PR is about the Application Emoji feature for Discord.

It's my first time submitting a related PR, and although I've ensured that the functionality should be working, there are probably still some issues. I'm also not very familiar with the docs.

I'm sorry for any inconvenience this may cause. If you have any suggestions or questions, please let me know, and I'll address them as soon as possible!

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@shiftinv shiftinv added t: enhancement New feature t: api support Support of Discord API features s: needs review Issue/PR is awaiting reviews labels Jul 19, 2024
@shiftinv shiftinv added this to the disnake v2.10 milestone Jul 19, 2024
disnake/emoji.py Outdated Show resolved Hide resolved
disnake/message.py Outdated Show resolved Hide resolved
@@ -817,7 +817,7 @@ def _convert_target_invite(self, target_id: int) -> Invite:
def _convert_target_webhook(self, target_id: int) -> Union[Webhook, Object]:
return self._webhooks.get(target_id) or Object(id=target_id)

def _convert_target_emoji(self, target_id: int) -> Union[Emoji, Object]:
def _convert_target_emoji(self, target_id: int) -> Union[Emoji, ApplicationEmoji, Object]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the name changed, I think it makes sense to update all existing references to Emoji with GuildEmoji

Additionally, since Union[GuildEmoji, ApplicationEmoji] appears so often, I suggest creating a type alias for it, perhaps AnyEmoji (probably in emoji.py)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah
Although I have already aliased Emoji to GuildEmoji, updating all existing references to Emoji with GuildEmoji is not a bad idea either.
I will create a type alias later. Thank you for your suggestion.

@NeloBlivion
Copy link

Hi, don't want to make a big deal out of this or anything but I'd appreciate it if you didn't blatantly copy-paste the code from Pycord-Development/pycord#2501 (including the mistakes in the first version), thanks 🥰

@HenrySpartGlobal
Copy link

HenrySpartGlobal commented Jul 23, 2024

Hi, don't want to make a big deal out of this or anything but I'd appreciate it if you didn't blatantly copy-paste the code from Pycord-Development/pycord#2501 (including the mistakes in the first version), thanks 🥰

Hi @NeloBlivion, Don't get me wrong, I understand your concern I believe the person who submitted the MR was trying to help, not being malicious. Both projects use the MIT License, and using and sharing code like this is allowed. It’s part of what makes open-source great!

Considering that both pycord and disnake are API wrappers for the same platform, it's natural that similar solutions might emerge, so I don't believe reinventing the wheel is necessary.

@NeloBlivion
Copy link

Hi @NeloBlivion, Don't get me wrong, I understand your concern I believe the person who submitted the MR was trying to help, not being malicious. Both projects use the MIT License, and using and sharing code like this is allowed. It’s part of what makes open-source great!

Considering that both pycord and disnake are API wrappers for the same platform, it's natural that similar solutions might emerge, so I don't believe reinventing the wheel is necessary.

Oh yeah, I get it - there's only so many differences you can get between libraries serving the same purpose, that were originally based off of the same project. I also enjoy looking at other library implementations to see how other people tackle new features (especially in updates like these where it can have greater implications on other structures).
It's not really a big issue and I don't expect this to close or anything, that being said i just found it strange that someone would directly copy a draft PR to pass off as their own work (obviously it's still under MIT license where this is perfectly fine, but still felt off to me)

anyway havefun i guess

@Mantouisyummy
Copy link
Author

Hi, don't want to make a big deal out of this or anything but I'd appreciate it if you didn't blatantly copy-paste the code from Pycord-Development/pycord#2501 (including the mistakes in the first version), thanks 🥰

Thank you for your reply. I apologize for making you think that I copied and pasted your code written in Pycord. In fact, I admired your work so much when I saw your code. Therefore, I studied some of your code and combined it with my own ideas to optimize it. If this has caused you any discomfort, I am truly sorry and ask for your understanding.

@Mantouisyummy
Copy link
Author

Since this code will be a breaking change for Disnake, I believe it will differ from Pycord's code, and I will continue to modify it. It is undeniable that my code was indeed inspired by yours, which may have unintentionally included some of your errors. For this, I apologize again. Your code is truly excellent.

@Enegg
Copy link
Contributor

Enegg commented Jul 26, 2024

Since this code will be a breaking change

What's the part that makes it a breaking change?

@EmmmaTech

This comment was marked as resolved.

@shiftinv
Copy link
Member

shiftinv commented Aug 7, 2024

Hi there, thanks for the PR. After some internal deliberation, I'm making the decision to close this.

While other commenters aren't wrong that the MIT license technically/legally allows this, and that both libraries themselves are naturally quite similar by virtue of being forks, I'm not entirely comfortable with merging this PR given the circumstances, sorry.

The base of this PR is quite clearly copied (at least partially) from the original linked PR - looking at a diff of diffs (select "Character" change highlighting) between this PR's base commit and the original PR's first commits up to the same date/time, some sections (e.g. is_usable or _remove_emoji) line up too well for them to just be "inspired" by another PR.
Aside from pre-existing differences between the two libraries, most of the other changes seen in that diff are formatting changes or renames, as far as I can tell. You've put in some refactoring work into this PR beyond that, which I don't want to just dismiss here, but it doesn't change the fact that the groundwork is the same.

Even if the license allows it, the fact that there wasn't a single reference to the original PR or any sort of attribution somewhat goes against the open source spirit, imho. The original author has made it pretty clear that they ultimately don't really approve of this - if this was coordinated with them, it would've probably been fine.
While maintaining an ideal "clean room" design for a library is probably unfeasible, I think everyone would still appreciate it if PR authors at least majorly author their own PRs.


In terms of implementation, it seems there's still some bikeshedding to be done. Before someone (you, or anyone else) gives implementing this another try, it might be worth talking about some design specifics first, either in a separate issue or #bikeshedding.

@shiftinv shiftinv closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: needs review Issue/PR is awaiting reviews t: api support Support of Discord API features t: enhancement New feature
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

6 participants